fix: solve potential deadlock for bsc p2p protocol handshake #1479
Conversation
NathanBSC
force-pushed
the
fix-bsc-p2p-protocal-handshake-potential-deadlock
branch
from
d9f401e
to
03f11f7
Compare
NathanBSC
changed the title
| @@ -458,7 +458,12 @@ func (ps *peerSet) registerPeer(peer *eth.Peer, ext *snap.Peer, diffExt *diff.Pe | |||
| eth.trustExt = &trustPeer{trustExt} | |||
| } | |||
| if bscExt != nil { | |||
| eth.bscExt = &bscPeer{bscExt} | |||
| if err := bscExt.Handshake(); err == nil { | |||
There was a problem hiding this comment.
Does this seem against the design of geth, as if every protocol follow the same pattern, the handshake of the different protocol will happen serially.
There was a problem hiding this comment.
the design of geth has no handshake exclude eth protocol,
waitBscExtension and registerBscExtension can end quickly
when handshake of sub protocol involved in above two , it' begin to slow down and lead to problem
| @@ -74,6 +86,11 @@ func MakeProtocols(backend Backend, dnsdisc enode.Iterator) []p2p.Protocol { | |||
| // Handle is the callback invoked to manage the life cycle of a `bsc` peer. | |||
| // When this function terminates, the peer is disconnected. | |||
| func Handle(backend Backend, peer *Peer) error { | |||
| select { | |||
| case <-peer.handshaked: | |||
There was a problem hiding this comment.
This seems not extensible, as the channel is only used by bsc protocol, it wont work if another extension is needed in the future.
There was a problem hiding this comment.
better snap and bsc share the same pattern.
There was a problem hiding this comment.
snap has no handshake, it's easy.
all problem begin from a sub protocol need handshake
There was a problem hiding this comment.
This seems not extensible, as the channel is only used by
bscprotocol, it wont work if another extension is needed in the future.
another sub protocol need handshake should have it's own handshaked channel
|
Try to illustrate how the deadlock happen:
|
|
I will shutdown this PR |
Description
fix potential deadlock by bsc p2p protocol handshake
Rationale
Process description:
timeout is 10 seconds
but ps.lock is got by other go routines, such as bscHandler.RunPeer and registerBscExtension
they are blocked in such code now
wait <- peerthen deadlock happens
Root cause :
func RunPeer is designed as not cost much time before.
but when handshake process added in, handshake also has a timeout,
so ethPeer have to wait more time than before, more close to timeout (10 seconds),
thus leading to improve chance of potential deadlock greatly.
Solve out:
we can move handshake of sub protocol such as bsc and diff
to behind handshake of main protocol
Example
add an example CLI or API response...
Changes
Notable changes: